Rebased onto master and fixed conflicts. Hopefully Travis-CI will be still OK with the change 👍

Travis tests were failing like this:

There was 1 failure:
1) ConceptMappingPropertyValueTest::testAsJskos
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'type' => Array (...)
     'toScheme' => Array (...)
     'from' => Array (...)
     'to' => Array (...)
+    'uri' => 'http://www.skosmos.skos/mapping/m1'
+    'notation' => null
+    'prefLabel' => 'Eel'
+    'description' => 'Exactly matching concepts in ...ulary.'
+    'hrefLink' => null
+    'lang' => 'en'
+    'vocabName' => 'Test ontology'
+    'typeLabel' => 'Exactly matching concepts'
/home/travis/build/NatLibFi/Skosmos/tests/ConceptMappingPropertyValueTest.php:203

It seems that ConceptMappingPropertyValueTest just has to be updated to match what the code does.

Tested this locally. Some observations:

1. The dynamic loading works fine when you do a full page load. But if you navigate to a concept page using the sidebar (alphabetical index, hierarchy, group index or new concepts list), then the mappings are not loaded at all. I bet this happens because when using the sidebar, the new concept page is loaded using an AJAX-style request, not a full page load.
2. The field labels ("EXACTLY MATCHING CONCEPTS" etc) and their descriptions (shown when hovering/tapping) are always in English, even when I'm using Finnish UI and content languages. They should be displayed in the UI language, like the rest of the field labels.
3. On pages for concepts which don't have any mappings, there is still a grey bar (div class=concept-appendix in the HTML source) displayed under the concept information. Before this PR, nothing was shown there.

I think that's enough review for this PR for now! Great work, it's advancing well though some things need to be fixed before this can be merged.

Rebased, updated, and tests passing now.

Good feedback on the Ajax responses! Hadn't looked at different ways the concept page could be used. Might be the case that the JavaScript used just needs to be loaded somewhere else.

Will test it during this (very rainy so far) weekend. Thanks!

The dynamic loading works fine when you do a full page load. But if you navigate to a concept page using the sidebar (alphabetical index, hierarchy, group index or new concepts list), then the mappings are not loaded at all. I bet this happens because when using the sidebar, the new concept page is loaded using an AJAX-style request, not a full page load.

Fixed in 5d75ca9. Interesting, I thought the whole page was being loaded when you clicked on the hierarchy node. Found where that magic takes place in docready.js ($(document).on('click', 'concept-hierarchy a')...), and changed the code to match what's done when the concept page is loaded.

Then later realized had to do the same for Concept, Hierarchy, and Groups. Should be working now.

The field labels ("EXACTLY MATCHING CONCEPTS" etc) and their descriptions (shown when hovering/tapping) are always in English, even when I'm using Finnish UI and content languages. They should be displayed in the UI language, like the rest of the field labels.

That's weird. Not sure what I changed that caused this regression. But I had a look at Finto, and the hover notes also appear always in English I think? https://finto.fi/yso/en/page/?clang=fi&uri=p13015

Or is it something else that needs to appear in Finnish instead of English. I mean the text EXACTLY MATCHING CONCEPTS and also what appears when I hover the mouse over that text.

On pages for concepts which don't have any mappings, there is still a grey bar (div class=concept-appendix in the HTML source) displayed under the concept information. Before this PR, nothing was shown there.

Oh, I see what you mean. Went to YSO groups, and clicked on 02. Philosophy and saw that spinner working its way, and then leaving the empty grey bar.

In the beginning, the component is hidden, and then it is displayed and a spinner starts so that the user can see there's something going on.

Then the next step was to populate the elements. Except when a concept had 0 properties returned, then the element was left empty. Mea culpa 🙂 . Now I've added some code to check for the length of properties returned from the server ajax response.

If no properties were returned, then the spinner is removed, and the element is hidden again. Only issue I see with this approach is that for a few seconds there will be a grey area, with the spinner. Is that a problem @osma ? I'm not sure if there's another way to fix this... as we don't access the properties mapping directly from the Model any longer 😞 What do you think?

Whew, addressed the points from the feedback I think. Waiting for Travis CI now, but if no issues found there, it should be ready for review again 🎉

Rebased 👍

Sorry @kinow, this has been waiting for another review way too long.

I fixed a minor merge conflict in concept-shared.twig and re-tested.

The AJAX style loading and extra grey bar issues are now solved, great!

There are still some issues with language support.

1. It appears that the lang parameter given to the mappings REST method is based on the UI language, not the content language. I think the content language should be used instead, so that the returned labels will be in the same language as the labels in the vocabulary. For example, if I'm browsing YSO Places with the UI language set to Finnish and content language set to English, and browse to the concept Africa (http://localhost/Skosmos/yso-paikat/fi/page/?clang=en&uri=p94080), the Wikidata link will be shown as "Afrikka" (in Finnish) instead of English as it should be.

2. The property labels "CLOSELY MATCHING CONCEPTS" etc. are still always displayed in English. The gettext calls made in ConceptMappingPropertyValue.php have no effect, probably because when they are called starting from RestController, the setLanguageProperties method is never called and so the locale settings are not in place. Even if they were, it's not clear how the mappings method would know what language to use.

I discussed the second problem with @kouralex and we came to the conclusion that it would be better to include all the possible property labels (and help texts / descriptions) in the property-mappings-template at the end of concept-shared.twig. There are only seven of them (see $MAPPING_PROPERTIES in Concept.php) so it would be possible to provide all of them in the template, pre-translated on the PHP/Twig side, and just pick the right one based on the property URI. This way some of the extra fields added to the JSKOS response could be dropped as well. What do you think @kinow?

Hi @osma ! Thanks for fixing the merge conflicts.

1. It appears that the lang parameter given to the mappings REST method is based on the UI language, not the content language. I think the content language should be used instead, so that the returned labels will be in the same language as the labels in the vocabulary (...)

Makes sense to me. Without looking at the code, I think we already have access to the vocabulary & its configuration and related objects. So it should be easily available somewhere I think 🤞

2. The property labels "CLOSELY MATCHING CONCEPTS" etc. are still always displayed in English. (...)

I discussed the second problem with @kouralex and we came to the conclusion that it would be better to include all the possible property labels (and help texts / descriptions) in the property-mappings-template at the end of concept-shared.twig. There are only seven of them (see $MAPPING_PROPERTIES in Concept.php) so it would be possible to provide all of them in the template, pre-translated on the PHP/Twig side, and just pick the right one based on the property URI. This way some of the extra fields added to the JSKOS response could be dropped as well. What do you think @kinow?

Sounds like a good workaround. Probably not likely to increase the number of property labels, so that should do.

Let me know you are planning on updating the PR with the workaround. Otherwise I can give it a try in the next days/weeks.

I fixed the first issue, it was a very simple fix (use clang instead of lang) though had to be applied in a few different places.

For the second one, I'd appreciate if you could take a look @kinow and update the PR.

We're planning a Skosmos 2.2 release (finally!), ideally this week, not sure if this can make it though.

Hi @osma

Oh, sorry about that. I should have done a better testing. Implemented your suggestion and I think it's working correctly.

I tested using the Finto installation, using English as interface language and content. Then switching content language to Finnish. Then switched the interface language to Finnish, and finally switched content from Finnish to English.

I had similar output in both — the difference was mainly some words/links were different, but I suspect that's just old data (e.g. instead of "Library of Congress Subject Headings", my YSO has "id.loc.gov").

PR rebased as well. Do you mind having another go at testing it, please?

Thanks!

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
7 Code Smells

No Coverage information
0.0% Duplication

Thanks @kinow! Much better now.
There was still the problem that the clang parameter for the mappings REST query wasn't actually used. I fixed that in the above commit.

Thanks @kinow! Much better now.
There was still the problem that the clang parameter for the mappings REST query wasn't actually used. I fixed that in the above commit.

Thanks @osma! It appeared to have worked before, funny.

There were some back-and-forth changes recently as we considered adding a clang parameter to the REST API - see #882 and #900. So if you based your fork somewhere in between the time these PRs got merged, the clang worked differently.

The new situation is different, as we genuinely need both lang and clang for the mappings method - lang for the property labels and clang for the labels of the mapping targets.

Merged, thanks a lot @kinow! Now we have to see how this affects the plugins/widgets and if we need to adjust the callback mechanisms in some cases. For example the Wikipedia widget used on YSO Places needs the graph of triples from Wikidata, which now arrives later than it used to.

Whoa, the Wikipedia widget seems to work, so you got the callbacks right too! Excellent!